Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move x-ditto-pre-authenticated and X-Forwarded-User to ingress.api.annotations #1787

Merged
merged 7 commits into from
Nov 3, 2023

Conversation

mladBlum
Copy link
Contributor

@mladBlum mladBlum commented Oct 30, 2023

resolves #1778

…roller to values.yaml config

Signed-off-by: Dominik Mlasko <[email protected]>
Copy link
Member

@thjaeckle thjaeckle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution, @mladBlum

Please update the Chart version in Chart.yaml to the next micro-version.

@thjaeckle thjaeckle added this to the 3.4.1 milestone Oct 31, 2023
Signed-off-by: Dominik Mlasko <[email protected]>
@thjaeckle
Copy link
Member

@mladBlum on "master" the version is already on version: 3.4.1 - sorry, did not see that.
In that case you'll have to bump to version: 3.4.2 - even if 3.4.1 is not yet released ..

@mladBlum
Copy link
Contributor Author

mladBlum commented Nov 2, 2023

@mladBlum on "master" the version is already on version: 3.4.1 - sorry, did not see that. In that case you'll have to bump to version: 3.4.2 - even if 3.4.1 is not yet released ..

Sorry, that's on me, I should have checked it myself.

Copy link
Member

@thjaeckle thjaeckle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - thanks a lot @mladBlum
I hope you verified that this works as you need it to .. ;)

I had to bump the version of helm/chart-testing-action - otherwise the chart linting was failing.

.github/workflows/helm-chart.yml Outdated Show resolved Hide resolved
.github/workflows/helm-chart.yml Outdated Show resolved Hide resolved
@thjaeckle thjaeckle merged commit 5452d9f into eclipse-ditto:master Nov 3, 2023
1 check passed
@RockyMM
Copy link
Contributor

RockyMM commented Nov 30, 2023

Hey @mladBlum, could you explain what were you trying to achieve with this change?

For us it broke the Ditto authentication as the pre-authenticated header is now not reaching the gateway pod. I am planning to make a PR that would revert this, but maybe there is an opportunity to achieve your goals and our goals at the same time? 😁

Oh, I see #1778. Hm, maybe there could be another way how to do it.

@thjaeckle
Copy link
Member

Oh, ouch .. really surprising to me how many of you don't use OAuth based authentication, but rely on that pre-authentication ..

@mladBlum
Copy link
Contributor Author

mladBlum commented Dec 1, 2023

Hey @mladBlum, could you explain what were you trying to achieve with this change?

For us it broke the Ditto authentication as the pre-authenticated header is now not reaching the gateway pod. I am planning to make a PR that would revert this, but maybe there is an opportunity to achieve your goals and our goals at the same time? 😁

Oh, I see #1778. Hm, maybe there could be another way how to do it.

Would be nice if we could solve this issue together. The same text you wrote applied for us before. I just placed the ditto headers into the ingress so that every ingress controller is working. I am really curious about the cause of your problem because this should be a more generic solution.

What is your setup?
Do you use the included ingress controller in the chart?

@RockyMM
Copy link
Contributor

RockyMM commented Dec 1, 2023

What is your setup? Do you use the included ingress controller in the chart?

Yes, we do. I am guessing you are not using the ingress controller included with the chart.

For us, what I see is that proxy_set_header directives ended up in the nginx.ingress.kubernetes.io/configuration-snippet of the ingresses and previously they were part of the location-snippet of the nginx-configuration ConfigMap. The end result is that the gateway is complaining with 401. When I returned the proxy_set_header directive manually to the ConfigMap the gateway was satisfied.

The included ingress controller is ingress-nginx/controller and we've pinned v1.9.1 because our kluster is v1.28.

In my perspective, the ConfigMap is the best place for this, so I will provide a rework of the Helm deployment that places these headers in the nginx-configuration ConfigMap. Hopefully you can work with that too.

@mladBlum
Copy link
Contributor Author

mladBlum commented Dec 1, 2023

What is your setup? Do you use the included ingress controller in the chart?

Yes, we do. I am guessing you are not using the ingress controller included with the chart.

For us, what I see is that proxy_set_header directives ended up in the nginx.ingress.kubernetes.io/configuration-snippet of the ingresses and previously they were part of the location-snippet of the nginx-configuration ConfigMap. The end result is that the gateway is complaining with 401. When I returned the proxy_set_header directive manually to the ConfigMap the gateway was satisfied.

The included ingress controller is ingress-nginx/controller and we've pinned v1.9.1 because our kluster is v1.28.

In my perspective, the ConfigMap is the best place for this, so I will provide a rework of the Helm deployment that places these headers in the nginx-configuration ConfigMap. Hopefully you can work with that too.

Ok, that's a little bit strange.
My assumption was that the included ingress controller also uses the proxy_set_header part in the ingress configuration. I'm very curious why he doesn't do that.
Probably we could add the headers in the place where they were before and also in the ingress itself. So both variants work. There are just a uncomfortable feelings to duplicate this configuration.

Please add me to the merge-request with the rework so i can check the changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Helm] Put x-ditto-pre-authenticated and X-Forwarded-User to ingress.api.annotations
3 participants